Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update mergebase in pr checker #10586

Merged
merged 2 commits into from
Mar 4, 2020
Merged

Update mergebase in pr checker #10586

merged 2 commits into from
Mar 4, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 3, 2020 •
edited
Loading

During PR check we should update the merge-base if it changes.

Fix #10502

@zeripath zeripath added this to the 1.12.0 milestone Mar 3, 2020
@lunny
Copy link
Member

lunny commented Mar 3, 2020

I think we also need a migration to fix the old PRs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 3, 2020
@codecov-io
Copy link

Codecov Report

Merging #10586 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10586      +/-   ##
==========================================
+ Coverage   43.71%   43.73%   +0.01%     
==========================================
  Files         585      585              
  Lines       82026    82026              
==========================================
+ Hits        35861    35876      +15     
+ Misses      41725    41712      -13     
+ Partials     4440     4438       -2
Impacted Files Coverage Δ
services/pull/check.go 55.48% <0%> (+5.48%) ⬆️
modules/indexer/stats/queue.go 62.5% <0%> (-18.75%) ⬇️
modules/indexer/stats/db.go 40.62% <0%> (-9.38%) ⬇️
modules/git/utils.go 65.67% <0%> (-4.48%) ⬇️
modules/git/repo.go 46.78% <0%> (-0.92%) ⬇️
services/pull/pull.go 35.88% <0%> (+1.17%) ⬆️
services/pull/temp_repo.go 31.62% <0%> (+2.56%) ⬆️
services/pull/patch.go 64.51% <0%> (+2.58%) ⬆️
modules/git/command.go 89.56% <0%> (+2.6%) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 009990b...e481c7d. Read the comment docs.

@zeripath
Copy link
Contributor Author

zeripath commented Mar 3, 2020

Hmm so on restart of Gitea all unmerged PRs should get their mergebase updated.

The issue with merged ones will be that it'll be difficult to decide what it should be after it's merged.

@zeripath
Copy link
Contributor Author

zeripath commented Mar 3, 2020

OK just doing some testing on my system this fix does appear to fix #10502

guillep2k
guillep2k approved these changes Mar 3, 2020
View reviewed changes
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I was expecting a PR with 17 files and 300+ lines changed. 😅

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 3, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 4, 2020
@6543
Copy link
Member

6543 commented Mar 4, 2020

Ping lgtm

@6543
Copy link
Member

6543 commented Mar 4, 2020

For migration: do we realy need one or just add this as option to the docktor?

@lunny
Copy link
Member

lunny commented Mar 4, 2020

Fixing old merged PR could be another PR.

@jolheiser jolheiser merged commit 4a2b76d into go-gitea:master Mar 4, 2020
@jolheiser
Copy link
Member

Please send backport. 🙂

@zeripath zeripath mentioned this pull request Mar 4, 2020
Merged
zeripath added a commit to zeripath/gitea that referenced this pull request Mar 4, 2020
@zeripath
Update mergebase in pr checker (go-gitea#10586)
7af3d84
@zeripath zeripath mentioned this pull request Mar 4, 2020
Merged
guillep2k added a commit that referenced this pull request Mar 4, 2020
@guillep2k
Update mergebase in pr checker (#10586) (#10605)
c9e4d7a
@zeripath zeripath mentioned this pull request Mar 4, 2020
Closed
@lunny
Copy link
Member

lunny commented Mar 5, 2020 •
edited
Loading

migrations/v128.go:79:fixMergeBase() [E] Unable to get merge base for PR ID 4314, Index 6 in ---/---. Error: exit status 128 - fatal: Not a valid object name fix-admin-2-master
& exit status 128 - fatal: ambiguous argument 'refs/heads/fix-admin-2-master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]'

found some logs from gitea.com

@zeripath
Copy link
Contributor Author

zeripath commented Mar 5, 2020

That means the base branch was deleted - so the merge base is impossible to recalculate. If the pr compare page was broken it will remain broken.

@zeripath
Copy link
Contributor Author

zeripath commented Mar 5, 2020

Interestingly - and concerningly - viewing that PR gives me a 500. That entry will be skipped by the migration - therefore it's not that v128.go has broken this - but rather that we're not handling deleted base branches very well.

@lafriks lafriks added the backport/done All backports for this PR have been created label Mar 5, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 approved these changes

@guillep2k guillep2k guillep2k approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

Squash merge message and compare incorrect following force-push
8 participants
@zeripath @lunny @codecov-io @6543 @jolheiser @guillep2k @lafriks @GiteaBot